chore: Add ADMIN_UPGRADE_TESTS label to test start-upgrade-test-admin #3355
Conversation
📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested Reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
julianrubino
added
no-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/e2e.yml (1)
26-26: Consider adding concurrency group for admin upgrade tests.While the workflow has a global concurrency group, consider adding a specific concurrency group for admin upgrade tests to prevent resource conflicts during parallel runs.
concurrency: group: e2e-${{ github.head_ref || github.sha }} cancel-in-progress: true + admin-upgrade: + group: admin-upgrade-${{ github.head_ref || github.sha }} + cancel-in-progress: trueAlso applies to: 246-249
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/e2e.yml(7 hunks)
🔇 Additional comments (3)
.github/workflows/e2e.yml (3)
118-118: LGTM: Output variable declaration is consistent with existing pattern.The ADMIN_UPGRADE_TESTS output variable is correctly declared following the established pattern in the matrix-conditionals job.
151-151: LGTM: Comprehensive event handling for ADMIN_UPGRADE_TESTS.The ADMIN_UPGRADE_TESTS flag is properly integrated across all relevant event contexts:
- Pull request label detection
- Release branch push events
- Scheduled runs
- Manual workflow dispatch
Also applies to: 182-182, 194-194, 208-208
246-249: Verify timeout configuration for admin upgrade tests.The new test target "start-upgrade-test-admin" is configured with a 40-minute timeout, which matches the timeout of similar upgrade tests like "start-upgrade-import-mainnet-test". However, let's verify if this timeout is sufficient.
Run this script to analyze historical run times of similar upgrade tests:
✅ Verification successful
Timeout configuration for admin upgrade tests is appropriate
The 40-minute timeout aligns with all other resource-intensive operations in the workflow, including similar upgrade tests, stateful data tests, and migration tests. This consistent pattern across comparable operations suggests the timeout is well-calibrated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check historical run times of upgrade tests to validate timeout configuration gh run list --workflow=e2e.yml --json conclusion,durationInMinutes,jobs --jq '.[] | select(.conclusion=="success") | .jobs[] | select(.name | contains("upgrade")) | {name: .name, duration: .durationInMinutes}' | head -n 10Length of output: 467
Script:
#!/bin/bash # Get historical run times for upgrade tests using available fields gh run list --workflow=e2e.yml --json conclusion,startedAt,updatedAt,jobs --jq ' .[] | select(.conclusion=="success") | .jobs[] | select(.name | contains("upgrade")) | { name: .name, duration: ( ( (fromdateiso8601(.updatedAt) - fromdateiso8601(.startedAt)) / 60 ) | floor ) } ' | head -n 10Length of output: 570
Script:
#!/bin/bash # Search for timeout configurations in upgrade-related jobs echo "Searching for upgrade test configurations:" rg "start-upgrade.*\n.*timeout-minutes:" .github/workflows/e2e.yml -A 1 echo -e "\nSearching for all timeout configurations:" rg "timeout-minutes:" .github/workflows/e2e.yml -B 2Length of output: 1305
There was a problem hiding this comment.
Kinda need to add a parameter on the e2e side to wait for the upgrade to complete before running the actual tests. start-upgrade-test-admin is a bit of a race condition.
There was a problem hiding this comment.
Isn't this test responsible for upgrading the network itself and then waiting for the upgrade to complete before running the actual tests?
https://github.com/zeta-chain/node/actions/runs/12795341388/job/35675082412?pr=3355
There was a problem hiding this comment.
I'm saying on the e2e side there's nothing to prevent the actual admin tests from starting to run before the network has actually upgraded.
Oh yeah we can't actually run this test in CI because people refuse to do automated authorization migrations 🎉 |
Description
Closes: 2708
This PR introduces the ADMIN_UPGRADE_TESTS label to trigger the start-upgrade-test-admin test in the GitHub Actions workflow.
This change ensures that tests specific to administrative upgrade scenarios can be executed as part of the CI/CD pipeline.
The label is added in the matrix-conditionals job and incorporated into the e2e test strategy matrix.
Key Changes:
Dependencies
None. This change is self-contained and builds upon the existing e2e workflow.
How Has This Been Tested?
Added the label and start-upgrade-test-admin are running.
Summary by CodeRabbit
New Features
Chores